Just make sure when we merge things into main, we can't break the existing tutorial we delivered for Roman asdf cutouts, or we need to modify the tutorial to work with any changes that impact it. Any previously delivered requirement may be tested with every Build.

Just make sure when we merge things into main, we can't break the existing tutorial we delivered for Roman asdf cutouts, or we need to modify the tutorial to work with any changes that impact it. Any previously delivered requirement may be tested with every Build.

I'll create a draft MR with roman_notebooks that we can mark as ready when this is merged in.

Thanks. Does the Roman Nexus pin on a particular astrocut version? If it does, we could also tell them not to update the version of astrocut in Nexus until we know the notebook tutorial is updated to support any backwards-incompatible changes to whatever the newer astrocut version is. I think long-term when the mission is live that's not a great way to go, since ideally the Nexus is always making the latest stable versions of packages available as quickly as possible to users, but while things need to be stable for testing prior to launch it might be useful / already being done?

I would assume that the Roman Nexus does have specific versions pinned in the same way that TIKE does. I'm not sure how to find them though, and I don't see anything in the roman_notebooks repository. I'll ask around!

While addressing Brian's comments, I've run into another design choice that I'm conflicted about.

Previously, fits_cut returns a list of file paths pointing to the cutouts. Because of this, I designed FITSCutout to return a list of filepaths if memory_only=False, and a list of memory objects if memory_only=True. However, the current version of asdf_cut always returns a Cutout2D object, regardless of whether the cutout file is written or not.

I personally prefer asdf_cut's method of always returning the memory objects rather than a list of file paths. However, I do want FITSCutout and ASDFCutout to be consistent with each other, and changing what fits_cut returns will be problematic.

I could add another parameter, something like return_paths, to control what is returned when files are written to disk. I'm still not sure what the default behavior should be though. Does anyone have thoughts?

In what ways would changing fits_cut return to work the same way as asdf_cut be problematic? Not knowing the codebase, would there be a path forward where, with extra work, fits_cut always returns a list of memory objects too, and then uses (the same, similar) function to choose to write it to a file at a particular location like presumably asdf_cut does now? Is fits_cut particularly user-facing, or would changing what it returns mostly require internal reconfiguring?

fits_cut is a user-facing function, and there wasn't an option to return memory objects before this re-org, so changing the default return value would have a large impact on any workflows that use it. fits_cut and asdf_cut are being maintained so that we have backwards compatibility, and we do note in the code (and will in the documentation) that we recommend using FITSCutout and ASDFCutout. Something I could do is add a return_paths parameter to FITSCutout with a default of False (so that it returns memory objects), but set the parameter to True when we call FITSCutout in fits_cut (so that paths are returned by default for that function only).

I've given this some more thought. Here are my two cents. IMO, as a user, the way that I'd want to work with FITSCut or ASDFCut is to be able to instantiate these objects, and then interact with them, i.e. create, inspect, and display the cutout, then write it out to a file of my choice. To support that, I would expose the _write_xxx methods to the user (make them public methods), expose the Cutout2D object as an attribute (and any other attributes that may be useful), and add new methods for quick inspection and display of the cutout overlay, wcs, etc. I would want to do things like quickly plot the original image and cutout overlay, plot just the cutout with wcs, inspect the wcs object, overlay the center target or other coordinates, run some validator that the cutout is correct, stitch cutouts together.

I also would want a convenience function that combines all those steps into a single function that I could run easily, for when I trust the code or don't care. It creates the cutout, saves the file, and returns me the filepath on disk. This is what the xxx_cut functions could be. They could retain the existing functionality of only returning the filepath(s). In this case we would change asdf_cut slightly. These functions could be loaded with all the user option kwargs to make choices and the conditional logic would live here, rather than pushed into the .cutout() method. Something of the sort...

def fits_cut(...):
    ff = FITSCut(..)
    cutout = ff.make_cutout(...).  # also sets ff.cutout 
    if as_fits:
       return ff.write_as_fits()
    elif as_img:
       return ff.write_as_img()
    elif memory_only:
       return cutout
    elif ....
    else:
      return ff.write_as_fits()

This could create some potential repeated boilerplate code between fits_cut, img_cut, and asdf_cut, so there may be a better way. What do you think of this approach? Or other thoughts on this?

I love this idea! It took me a while to change things around, but I got something working that majorly decreases the complexity of these classes by moving the logic for writing files into separate functions (write_as_image, write_as_asdf, and write_as_fits).

Added attributes for ASDFCutout:

• cutouts_by_file: dict of Cutout2D objects stored by input filepath
• cutouts: list of Cutout2D objects
• asdf_cutouts: list of asdf.AsdfFile objects
  • "Lazy loaded" so this attribute isn't assigned until called
• fits_cutouts: list of fits.HDUList objects
  • "Lazy loaded" so this attribute isn't assigned until called

Added attributes for FITSCutout:

• cutouts_by_file: dict of numpy arrays stored by input filepath
  • Eventually, I want the values here to be Cutout2D objects like in ASDFCutout
• hdu_cutouts_by_file: dict of ImageHDU objects stored by input filepath
• fits_cutouts: list of fits.HDUList objects
  • "Lazy loaded" so this attribute isn't assigned until called

For ImageCutout, I added an image_cutouts property which calls the function get_image_cutouts that accepts the normalization parameters as input and returns the image cutouts as memory objects.

You had some great ideas for other features like plotting, exposing a WCS, etc. I'm still thinking about how features like these will work best given that the classes take multiple input files and can generate a lot of different cutouts at once. I could save the WCS for every cutout, but that seems like a waste of resources. I could also add these things as separate functions, but require a single cutout to be passed in as input? I'm not sure.

I think I came up with a pretty decent solution! I added a nested class, CutoutInstance to FITSCutout that "mimics" the attributes of Cutout2D. This eliminates some of the complexity in handling different types of objects between the cutout classes. Eventually, when Cutout2D is updated and a new version of Astropy is released, CutoutInstance can be replaced with Cutout2D.

FITSCutout.cutouts_by_file is a dictionary where the keys are input filenames and the values are lists of CutoutInstance objects. Using this attribute, users can access individual cutout characteristics like data, WCS, and cutout slices.

Another change I made is how FITSCutout handles empty cutouts when single_outfile=True. In the current implementation, ImageHDUs with empty data are still added to the output HDUList. I think it's more performant and user-friendly to skip these HDUs and just make the output file with the files and extensions that have cutout data. I added a warning message that logs when an input file has no cutout data and will be skipped. However, this is a change to how the fits_cut function currently behaves, and in rare cases, it could break a user's code. I'm open to any thoughts on this! If we do go through with the change, I will make sure that the documentation reflects it.

I'll respond briefly to your comments here, and then do a full PR review in a bit.

You had some great ideas for other features like plotting, exposing a WCS, etc. I'm still thinking about how features like these will work best given that the classes take multiple input files and can generate a lot of different cutouts at once. I could save the WCS for every cutout, but that seems like a waste of resources. I could also add these things as separate functions, but require a single cutout to be passed in as input? I'm not sure.

Yeah I'm not sure if we need to have all class methods support batch mode. I imagine using the classes themselves mostly on a per image/cutout basis for inspection, where each method works on a single cutout.

I hadn't fully appreciated nor thought about only working in a batch mode. We may want to brainstorm a bit more on some workflows. I feel most use cases of batching cutouts would be to run them via the convenience functions to return to later, and less via the interactive class instance itself. One scenario might be a user has a bunch of images they want to make cutouts from. They instantiate one in FITSCut to test and inspect the cutout, check settings etc. They then run the whole set in batch mode. Later they want to spot check some of the cutout files. It would be great if they could reload a cutout file via FITSCut after-the-fact to inspect and display.

This may change the scope of what Astrocut should do though. If they don't already, I think the cutout files should contain metadata about the original source file and the cutout info in its header. In principle then one could do something like ff= FITSCut.from_cutout('path/to/cutout_file.fits') to reload it into ff.cutout then use the class methods to inspect it. It could even reload the original source file if available. But this workflow may require some more thought. All this would be for a future PR.